Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Put query data into the data parameter of the fixture function. #79

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mtnt
Copy link

@mtnt mtnt commented May 16, 2018

No description provided.

@elisherer
Copy link
Contributor

I think if look through superagent code you will find somewhere that query object in this. that way eliminate the need for qs (just a suggestion)

@mtnt
Copy link
Author

mtnt commented May 17, 2018

@elisherer there is. It's in the qs key and it is cleared just after stringifying. Besides it is an inner property that I would not prefer to use =)

NB: I removed accidentally added commit Dirty

return data;
}

const parts = url.split('?');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't it be simpler to check includes?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parts[1] is used further

@@ -1,3 +1,6 @@
import qs from 'qs';
import {isEmpty, isNil} from 'lodash';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think lodash is a big dependency here, will require consumers to have some lind of lodash optimization. better make those checks part of the code or change the imports to direct ones.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really think there are totally all projects have lodash) But ok, I removed it)

@mtnt
Copy link
Author

mtnt commented May 23, 2018

@elisherer could you already merge or reject it?)

@DevSide
Copy link
Contributor

DevSide commented Jun 6, 2018

Hi, thanks for your contribution @mtnt.

I wouldn't be exclusive between post data and query string because it is possible to have query params in a POST url 😄.

If I well understand what you need, you can use Regexp capturing groups in the pattern.

// Example 1, the pattern is stricted and the query params order is known
{
  pattern: 'https://domain.example?limit=(\d+)&offset=(\d+)',
  fixtures: match => {
    const [, limit, offset] = match;
    // ...
  }
}

// Example 2, the pattern matches every requests with query params
[{
  pattern: 'https://domain.example?(.*)',
  fixtures: match => {
    const { limit, offset } = qs(match[1]);
    // ...
    // may handle 404 somewhere
  }
}]

@mtnt
Copy link
Author

mtnt commented Jun 6, 2018

@DevSide hmm, I got your point... Yeah, I did not think about usage query params in a POST...
But. Don`t you think the current behavior is much inconsistent?

Maybe the data should be an object with query and data keys in this case?

PS yeah, the regexp is that what I currently use for this purpose.

@mtnt
Copy link
Author

mtnt commented Jun 6, 2018

Usage of a regexp is not convenient: actual params order may be different with a pattern. Much more useful way is to get somehow a package of parsed data.

UPD: it`s possible to use "?(.*)" and parse it - in this case each user should reverse engineer the superagent and clarify the qs library usage for stringifying params.

@DevSide
Copy link
Contributor

DevSide commented Jun 6, 2018

path-to-regex could be better choice I guess but not sure it handles query params.

@mtnt
Copy link
Author

mtnt commented Jun 6, 2018

Oh, I`m sorry, I forgot one more thing: passing complex params such as objects and arrays are converted not like json: query({foo: {a: 1, b: 2}}) will be ...?foo[a]=1&foo[b]=2. It is little bit difficult to parse it using regexp =(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants